Simplify filterText computation + fixes
#1414
Conversation
| if filterText == "" { | ||
| filterText = getFilterText(file, position, insertText, name, isMemberCompletion, isSnippet, wordStart) | ||
| } | ||
| if isMemberCompletion && !isSnippet { |
There was a problem hiding this comment.
This code here was removed because it mostly overlapped with getFilterText above.
| m() { | ||
| this.; |
There was a problem hiding this comment.
This results from fixing the scanner bug.
| @@ -2127,41 +2143,45 @@ func getFilterText( | |||
| // In which case we want to insert a bracket accessor but should use `.abc` as the filter text instead of | |||
There was a problem hiding this comment.
This is sort of an aside, but I don't understand why conceptually the "access operator" (the . or ?.) is part of the filter text at all.
There was a problem hiding this comment.
The client will rank the completion items by comparing the filter text with some suffix of the existing test at the completions position. The suffix is usually the word touching the position, but may be a custom range when the item specifies a text edit.
Given that, we want to use the filter text to avoid the client overly de-ranking or omitting completions that have to modify some of the text that the user already typed.
For instance, if we have this:
declare const obj: { "ab c": string };
obj.a|The completion item for ab c will need to insert ["ab c"], and it will specify a range in the text edit covering .a, but we want it to be ranked by the client the same as a regular property should, so the client should compare .a (text at current position that the item will replace) with .ab c, instead of ["ab c"], because ["ab c"] doesn't really match .a.
The access operator is there because in the situations where we use it in the filter text, it is part of the text that will be replaced if you accept the completion item, so it is part of what will be compared with the filter text.
That said, it's possible there are some cases where we currently set the filter text that could be optimized away.
There was a problem hiding this comment.
I see, so this is entirely because the filter text is matched up against text in the replacement range. If there was a distinct way to send back what we calculated as the "effective search text", we wouldn't have to hack around with the operator itself.
Funny enough, you can see how this sort of breaks down if you insert a newline or comment after a dot.
| } | ||
| s.token = ast.KindPrivateIdentifier |
There was a problem hiding this comment.
Seems fine, but why did we need this? Or, why is Strada able to get away without this?
There was a problem hiding this comment.
The new code does what Strada does. The code before this PR seems like a porting bug.
This PR simplifies the computation of a completion item's
filterTextwhich was ported from the VSCode TS extension. There were originally two pieces of code wherefilterTextwas computed, and I think one of those was mostly unnecessary, so I removed it and simplified the rest of the code.The code I removed used to set
filterTextfor all dot completions, which wasn't needed, so the Corsa tests have been updated to reflect that, and a bunch of tests ported from Strada now pass as well because we no longer setfilterTextin most cases.I also added tests for all the cases where I think
filterTextis necessary, since this piece of code is new and so not covered by Strada tests.This PR also fixes two porting bugs, one in the parser and one bug in the scanner, and some minor completion bugs.